[KeyInstr] Enable -gkey-instructions by default if optimisations are enabled #149509
Conversation
…enabled Key Instructions improves the optimized-code debug-stepping experience in debuggers that use DWARF's `is_stmt` line table register to determine stepping behaviour. The feature can be disabled with `-gno-key-instructions`. RFC: https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Orlando Cazalet-Hyams (OCHyams) ChangesThat's enabling Clang's -gkey-instructions, cc1's -gkey-instructions remains off by default. Key Instructions improves the optimized-code debug-stepping experience in debuggers that use DWARF's The feature can be disabled with RFC: https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668 Questions: LLDB - I believe LLDB ignores Clang's Clang languages/extensions - I've implemented support for C/C++, but I've not tested or specifically implemented anything for other languages (?) or extensions (?) like CUDA. Is there a good way to filter for this in the driver code in this patch? Full diff: https://github.com/llvm/llvm-project/pull/149509.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 8880c9375143f..38eebbd6f9b5f 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -4611,8 +4611,14 @@ renderDebugOptions(const ToolChain &TC, const Driver &D, const llvm::Triple &T,
CmdArgs.push_back("-gembed-source");
}
+ // Enable Key Instructions by default if optimisations are enabled and
+ // we're emitting DWARF.
+ Arg *OptLevel = Args.getLastArg(options::OPT_O_Group);
+ bool KeyInstructionsOnByDefault =
+ EmitDwarf && OptLevel && !OptLevel->getOption().matches(options::OPT_O0);
if (Args.hasFlag(options::OPT_gkey_instructions,
- options::OPT_gno_key_instructions, false))
+ options::OPT_gno_key_instructions,
+ KeyInstructionsOnByDefault))
CmdArgs.push_back("-gkey-instructions");
if (EmitCodeView) {
diff --git a/clang/test/DebugInfo/KeyInstructions/flag.cpp b/clang/test/DebugInfo/KeyInstructions/flag.cpp
index e34faa6cbb347..05a34ac670feb 100644
--- a/clang/test/DebugInfo/KeyInstructions/flag.cpp
+++ b/clang/test/DebugInfo/KeyInstructions/flag.cpp
@@ -1,7 +1,5 @@
// RUN: %clang -### -target x86_64 -c -gdwarf -gkey-instructions %s 2>&1 | FileCheck %s --check-prefixes=KEY-INSTRUCTIONS
// RUN: %clang -### -target x86_64 -c -gdwarf -gno-key-instructions %s 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
-//// Default: Off.
-// RUN: %clang -### -target x86_64 -c -gdwarf %s 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
//// Help.
// RUN %clang --help | FileCheck %s --check-prefix=HELP
@@ -23,3 +21,34 @@ void f() {}
// RUN: %clang_cc1 %s -triple x86_64-linux-gnu -gkey-instructions -debug-info-kind=line-tables-only -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
// SMOKETEST-ON: keyInstructions: true
// SMOKETEST-ON: atomGroup: 1
+
+//// Enable Key Instructions by default if optimisations are enabled and we're
+//// emitting DWARF.
+////
+//// | opt level | -gkey-instructions | feature |
+//// | 0 | no | off |
+//// | 0 | yes | on |
+//// | >=1 | no | on |
+//// | >=1 | yes | on |
+//// | >=1 | no & no -g flags | off |
+//// | >=1 | no & emit codeview | off |
+//
+// RUN: %clang %s -target x86_64 -gdwarf -gmlt -### 2>&1 | FileCheck %s --check-prefix=NO-KEY-INSTRUCTIONS
+// RUN: %clang %s -target x86_64 -gdwarf -gmlt -gkey-instructions -### 2>&1 | FileCheck %s --check-prefix=KEY-INSTRUCTIONS
+// RUN: %clang %s -O0 -target x86_64 -gdwarf -gmlt -### 2>&1 | FileCheck %s --check-prefix=NO-KEY-INSTRUCTIONS
+// RUN: %clang %s -O0 -target x86_64 -gdwarf -gmlt -gkey-instructions -### 2>&1 | FileCheck %s --check-prefix=KEY-INSTRUCTIONS
+// RUN: %clang %s -O1 -target x86_64 -gdwarf -gmlt -### 2>&1 | FileCheck %s --check-prefix=KEY-INSTRUCTIONS
+// RUN: %clang %s -O1 -target x86_64 -gdwarf -gmlt -gkey-instructions -### 2>&1 | FileCheck %s --check-prefix=KEY-INSTRUCTIONS
+// RUN: %clang %s -O1 -target x86_64 -### 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
+// RUN: %clang %s -O1 -target x86_64 -gcodeview -gmlt -### 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
+//
+// RUN: %clang %s -target x86_64 -gdwarf -gmlt -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-OFF
+// RUN: %clang %s -target x86_64 -gdwarf -gmlt -gkey-instructions -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
+// RUN: %clang %s -O0 -target x86_64 -gdwarf -gmlt -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-OFF
+// RUN: %clang %s -O0 -target x86_64 -gdwarf -gmlt -gkey-instructions -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
+// RUN: %clang %s -O1 -target x86_64 -gdwarf -gmlt -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
+// RUN: %clang %s -O1 -target x86_64 -gdwarf -gmlt -gkey-instructions -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
+// RUN: %clang %s -O1 -target x86_64 -S -emit-llvm -o - | FileCheck %s --check-prefixes=SMOKETEST-OFF,SMOKETEST-NO-DEBUG
+// RUN: %clang %s -O1 -target x86_64 -gcodeview -gmlt -S -emit-llvm -o - | FileCheck %s --check-prefixes=SMOKETEST-OFF
+// SMOKETEST-NO-DEBUG: llvm.module.flags
+// SMOKETEST-NO-DEBUG-NOT: DICompileUnit
|
|
Ping (should I move the question to discourse?) |
|
A summary of DWARF file size/section size changes (how much change as a % of total file size in object and linked executables, as a % of aggregate DWARF section sizes in object and linked executables, and as a % of the specific DWARF section sizes (eg: {.rela,}.debug_line changes by x%) - with specific debug info flags (my preference would be for -fno-standalone-debug -gz=zstd with crel, but I understand if that's not your deployment target, etc, it's less relevant)) and which debuggers are effected in what ways (some discussion of gdb would be nice). If this has a cost and no benefit for lldb, seems like maybe we'd want to keep it off under lldb tuning. If it has a cost and no benefit to gdb - then I'd probably say off-by-default, on-for-SCE. |
|
Thanks for taking a look!
Here's bloaty figures for some CTMark builds ( I'm only including
There's a compile time cost to enabling the feature (0.52% on compile-time-tracker) which was included in the compile-time-cost tables in this post which explains where we put work in to "pay for" the cost of the feature (and we have longer term general improvements we'd like to make). I'm happy to get more numbers if there're any other configurations you'd like to see.
SCE (full disclosure - most tested) and GDB (*) and both work with the feature (both provide the promised improved optimisied-code stepping), but LLDB doesn't yet. I'd still like to gently suggest we enable it for LLDB too for uniformity, especially as we've tried to pay some of the costs in other areas, so the net "feature doesn't exist" -> "feature on by default" cost is < 0.3% increased (*) Checked with 15.0.50 and 12.0.90. GCC's |
|
Cool - thansks for all the details. I'm fine with making the change in general then, but should get some thougths from the apple folks, probably @JDevlieghere or @adrian-prantl |
|
I think you've all gone above and beyond to pay for the (marginal) cost increase from this feature, and we should go ahead and land this. Thanks for all the hard work and data gathering! |
That's enabling Clang's -gkey-instructions, cc1's -gkey-instructions remains off by default.
Key Instructions improves the optimized-code debug-stepping experience in debuggers that use DWARF's
is_stmtline table register to determine stepping behaviour.The feature can be disabled with
-gno-key-instructions.RFC: https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668
Questions:
LLDB - I believe LLDB ignores Clang's
is_stmts for stepping, but I don't know whether movingis_stmts around in ways LLDB hasn't seen before (at least as output from Clang) is going to have any unexpected consequences. cc @adrian-prantl @JDevlieghere for opinions - what testing and checking would be needed / possible here, to be confident that this doesn't break optimized code debugging for LLDB?Clang languages/extensions - I've implemented support for C/C++, but I've not tested or specifically implemented anything for other languages (?) or extensions (?) like CUDA. Is there a good way to filter for this in the driver code in this patch?